C# 14: Support extension types.#21220
Conversation
csharp/extractor/Semmle.Extraction.CSharp/CodeAnalysisExtensions/SymbolExtensions.cs
Fixed
Show fixed
Hide fixed
6218b5c to
5c44c08
Compare
csharp/extractor/Semmle.Extraction.CSharp/CodeAnalysisExtensions/SymbolExtensions.cs
Fixed
Show fixed
Hide fixed
…meter and make it possible to specify a parameter position offset.
a03f985 to
2a9166f
Compare
…tic generated methods with invocation of extension type member.
extension blocks.extension types.
2a9166f to
02e4a8b
Compare
|
@hvitved : The PR is ready for review. However, I haven't added any upgrade/downgrade scripts yet in case the review leads to a change in the DB scheme. Review on commit by commit basis is encouraged. |
hvitved
left a comment
There was a problem hiding this comment.
Impressive work! Some mostly minor comments
| | @xmllocatable | @asp_element | @namespace | @preprocessor_directive; | ||
|
|
||
| @declaration = @callable | @generic | @assignable | @namespace; | ||
| @declaration = @callable | @generic | @assignable | @namespace | @extension_type; |
There was a problem hiding this comment.
Is this needed? I would think that @extension_type is part of @type, which is part of @generic.
| { | ||
| /// <summary> | ||
| /// Synthetic parameter for extension methods declared using the extension syntax. | ||
| /// That is, we add a synthetic parameter s to IsValid in the following example: |
There was a problem hiding this comment.
backticks around s and IsValid makes this easier to parse.
| switch (ExtensionParameter.RefKind) | ||
| { | ||
| case RefKind.Ref: | ||
| return Parameter.Kind.Ref; | ||
| case RefKind.In: | ||
| return Parameter.Kind.In; | ||
| case RefKind.RefReadOnlyParameter: | ||
| return Parameter.Kind.RefReadOnly; | ||
| default: | ||
| return Parameter.Kind.None; | ||
| } |
There was a problem hiding this comment.
Should we share this logic with the logic in Parameter.cs?
| /// Returns true if this method is a compiler-generated extension method, | ||
| /// and outputs the original extension method declaration. | ||
| /// </summary> | ||
| public static bool TryGetExtensionMethod(this IMethodSymbol method, out IMethodSymbol? declaration) |
There was a problem hiding this comment.
Why not simply public static IMethodSymbol? TryGetExtensionMethod(this IMethodSymbol method)?
| * Either an extension method (`ExtensionMethod`), an extension operator | ||
| * (`ExtensionOperator`) or an extension accessor (`ExtensionAccessor`). | ||
| */ | ||
| abstract class ExtensionCallable extends Callable { |
There was a problem hiding this comment.
We don't want to expose abstract classes, so better to (1) rename it to ExtensionCallableImpl, (2) move it inside internal/Callable.qll, (3) update all existing references to ExtensionCallableImpl, and (4) add here final class ExtensionCallable = ExtensionCallableImpl.
| Call getACall() { this = result.getTarget() } | ||
|
|
||
| /** Holds if this callable is declared in an extension type. */ | ||
| predicate isInExtension() { this.getDeclaringType() instanceof ExtensionType } |
There was a problem hiding this comment.
If you move this inside the Declaration class, it can be used also in the charpreds of ExtensionProperty and ExtensionAccessor.
| * Either a classic extension method (`ClassicExtensionMethod`) or an extension | ||
| * type extension method (`ExtensionTypeExtensionMethod`). | ||
| */ | ||
| abstract class ExtensionMethod extends ExtensionCallable, Method { |
| * ``` | ||
| */ | ||
| class SyntheticExtensionParameterAccess extends ParameterAccess { | ||
| private Parameter p; |
There was a problem hiding this comment.
No need to have this as a field.
| - ["My.Qltest", "K", false, "GetMyFieldOnSyntheticField", "()", "", "Argument[this].SyntheticField[My.Qltest.K.MySyntheticField2].Field[My.Qltest.K.MyField]", "ReturnValue", "value", "manual"] | ||
| - ["My.Qltest", "Library", false, "SetValue", "(System.Object)", "", "Argument[0]", "Argument[this].SyntheticField[X]", "value", "dfc-generated"] | ||
| - ["My.Qltest", "Library", false, "GetValue", "()", "", "Argument[this].SyntheticField[X]", "ReturnValue", "value", "dfc-generated"] | ||
| - ["My.Qltest", "TestExtensions+extension(Object)", false, "Method1", "(System.Object)", "", "Argument[0]", "ReturnValue", "value", "manual"] |
There was a problem hiding this comment.
This ought to use the FQN of the type being extended, i.e. TestExtensions+extension(System.Object)
| catch | ||
| { | ||
| // If anything goes wrong, fall back to the unbound declaration. | ||
| return unboundDeclaration; | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
| if (parameter.Ordinal == 0) | ||
| { | ||
| if (parameter.ContainingSymbol is IMethodSymbol method && method.IsExtensionMethod) | ||
| { | ||
| return Parameter.Kind.This; | ||
| } | ||
| } |
Check notice
Code scanning / CodeQL
Nested 'if' statements can be combined Note
In this PR we introduce support for
extensiontypes (blocks). The feature is explained in detail here.The feature generalises extensions to operators and properties.
Here is a small example of an declaration and how it can be invoked.
It turns out that Roslyn generates a static method corresponding to the extension method. To avoid extracting multiple methods with identical bodies (which would further complicate the QL implementation) we
IsValid()method, which has the qualified nameMyExtensions.extension(string).IsValid.stoIsValid()corresponding to the receiver parameters. This needs to be synthesised as we can't re-use the parameter from the extension declaration.MyExtensions.IsValidwithMyExtensions.extensions(string).IsValid.DCA looks good.
dotnet/runtimeproject. These extraction errors are because some of the extensions (that we now extract) adds user-defined compound assignment operators (described here), which are not supported yet.